Automated Test: feature-authz-crypto-implementation #322
Conversation
…tory closes #33831 Signed-off-by: mposolda <mposolda@gmail.com>
📝 WalkthroughWalkthroughThe changes introduce ECDSA signature encoding/decoding capabilities to the authorization client module via new ASN.1 utilities and a CryptoProvider implementation. A provider ordering mechanism is added to the CryptoProvider interface and implemented across existing crypto providers. Test dependencies and initialization hooks are added to support the new functionality. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ❌ 2❌ Failed checks (2 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In
`@authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/ASN1Decoder.java`:
- Around line 127-167: The method ASN1Decoder.readLength() currently returns -1
to signal indefinite-length encoding (0x80), but callers like readSequence() and
readInteger() do not handle -1 and can misbehave; change readLength() to throw
an IOException when encountering 0x80 (indefinite-length) instead of returning
-1 so callers get a clear error path consistent with other validation checks;
update any error message to indicate "indefinite-length encoding (0x80) not
supported" and rely on existing exception handling in readSequence() and
readInteger() to avoid negative array sizes or silent empty reads.
In
`@authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/AuthzClientCryptoProvider.java`:
- Around line 114-115: Remove the two dead ASN1Encoder.create().write(...) calls
that discard results; in AuthzClientCryptoProvider replace the redundant
ASN1Encoder.create().write(rBigInteger) and
ASN1Encoder.create().write(sBigInteger) invocations with only the actual
encoding path that uses the encoder instance later (the code that performs the
real encoding for rBigInteger and sBigInteger), ensuring you keep the working
encoder usage and remove the discarded temporary encoder calls.
In
`@authz/client/src/test/java/org/keycloak/authorization/client/test/ECDSAAlgorithmTest.java`:
- Line 54: Rename the misspelled variable asn1Des to asn1Der in
ECDSAAlgorithmTest.java where the result of
prov.getEcdsaCryptoProvider().concatenatedRSToASN1DER(rsConcat,
algorithm.getSignatureLength()) is assigned; update all usages of asn1Des in the
test to use asn1Der so the variable name correctly reflects the ASN.1 DER format
returned by concatenatedRSToASN1DER.
In `@common/src/main/java/org/keycloak/common/crypto/CryptoIntegration.java`:
- Around line 64-70: The logging uses logger.debugf with an unsanitized format
string and the loop leaves a trailing comma; in CryptoIntegration when handling
foundProviders, replace logger.debugf(builder.toString()) with logger.debug(...)
and build the provider list without a trailing ", " (e.g., append separators
conditionally or use String.join over
foundProviders.stream().skip(1).map(p->p.getClass().getName()).collect(...)) so
the output is printed raw and has no trailing separator.
In `@common/src/main/java/org/keycloak/common/crypto/CryptoProvider.java`:
- Around line 39-44: The new non-default method on the public interface
CryptoProvider (int order()) will break external implementations; change it to a
default method (default int order()) that returns a safe default (e.g., 0) so
existing third-party implementations remain compatible while internal classes
(AuthzClientCryptoProvider, DefaultCryptoProvider, FIPS1402Provider,
WildFlyElytronProvider) can keep their explicit overrides with 100/200.
🧹 Nitpick comments (4)
authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/ASN1Decoder.java (1)
49-63: Sequence element length tracking may miscount with long-form tags/lengths.In
readSequence(), the contentlengthreturned byreadLength()represents the total bytes of the sequence's content (all child TLVs combined). The loop subtractsbytes.length(the full TLV of each child element returned byreadNext()) which is correct.However,
readNext()re-reads the tag and length by resetting the stream. This is clever but tightly couples the correctness tocountbeing accurate across all internal reads. Consider adding a brief comment explaining this invariant for maintainability.authz/client/src/test/java/org/keycloak/authorization/client/test/ECDSAAlgorithmTest.java (1)
41-43: Single default-size EC key used for all three algorithm tests.
KeyPairGenerator.getInstance("EC").genKeyPair()uses the JDK's default curve (typically P-256). ES384 expects P-384 and ES512 expects P-521. The round-trip encoding test still passes becauseintegerToBytespads/trims, but R and S values will always be ≤32 bytes, so the larger-format paths are only exercised with zero-padded data rather than realistic large-coordinate values.Consider generating an appropriately-sized key per algorithm (e.g., pass curve name to
ECGenParameterSpec) to improve coverage.authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/AuthzClientCryptoProvider.java (2)
100-164: Consider caching theECDSACryptoProviderinstance.
getEcdsaCryptoProvider()allocates a new anonymous class on every call. Since it's stateless, returning a singleton field avoids needless allocations on hot paths.
60-67:getBouncyCastleProvider()returns the default KeyStore provider, not BouncyCastle.This is understandable given the minimal-provider intent, but it may confuse callers that expect an actual BC
Provider. A brief Javadoc note clarifying the deliberate non-BC behavior would help future readers.
| int readLength() throws IOException { | ||
| int length = read(); | ||
| if (length < 0) { | ||
| throw new EOFException("EOF found when length expected"); | ||
| } | ||
|
|
||
| if (length == 0x80) { | ||
| return -1; // indefinite-length encoding | ||
| } | ||
|
|
||
| if (length > 127) { | ||
| int size = length & 0x7f; | ||
|
|
||
| // Note: The invalid long form "0xff" (see X.690 8.1.3.5c) will be caught here | ||
| if (size > 4) { | ||
| throw new IOException("DER length more than 4 bytes: " + size); | ||
| } | ||
|
|
||
| length = 0; | ||
| for (int i = 0; i < size; i++) { | ||
| int next = read(); | ||
|
|
||
| if (next < 0) { | ||
| throw new EOFException("EOF found reading length"); | ||
| } | ||
|
|
||
| length = (length << 8) + next; | ||
| } | ||
|
|
||
| if (length < 0) { | ||
| throw new IOException("corrupted stream - negative length found"); | ||
| } | ||
|
|
||
| if (length >= limit) // after all we must have read at least 1 byte | ||
| { | ||
| throw new IOException("corrupted stream - out of bounds length found"); | ||
| } | ||
| } | ||
|
|
||
| return length; | ||
| } |
There was a problem hiding this comment.
Indefinite-length encoding (0x80) is recognized but not safely handled by callers.
readLength() returns -1 for indefinite-length encoding. In readSequence(), this silently returns an empty list (the while (length > 0) loop never executes). In readInteger(), it causes a NegativeArraySizeException from new byte[-1]. Since DER prohibits indefinite-length encoding, throwing an IOException here instead of returning -1 would be more defensive and consistent with the other error paths in this method.
Proposed fix
if (length == 0x80) {
- return -1; // indefinite-length encoding
+ throw new IOException("Indefinite-length encoding is not supported in DER");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| int readLength() throws IOException { | |
| int length = read(); | |
| if (length < 0) { | |
| throw new EOFException("EOF found when length expected"); | |
| } | |
| if (length == 0x80) { | |
| return -1; // indefinite-length encoding | |
| } | |
| if (length > 127) { | |
| int size = length & 0x7f; | |
| // Note: The invalid long form "0xff" (see X.690 8.1.3.5c) will be caught here | |
| if (size > 4) { | |
| throw new IOException("DER length more than 4 bytes: " + size); | |
| } | |
| length = 0; | |
| for (int i = 0; i < size; i++) { | |
| int next = read(); | |
| if (next < 0) { | |
| throw new EOFException("EOF found reading length"); | |
| } | |
| length = (length << 8) + next; | |
| } | |
| if (length < 0) { | |
| throw new IOException("corrupted stream - negative length found"); | |
| } | |
| if (length >= limit) // after all we must have read at least 1 byte | |
| { | |
| throw new IOException("corrupted stream - out of bounds length found"); | |
| } | |
| } | |
| return length; | |
| } | |
| int readLength() throws IOException { | |
| int length = read(); | |
| if (length < 0) { | |
| throw new EOFException("EOF found when length expected"); | |
| } | |
| if (length == 0x80) { | |
| throw new IOException("Indefinite-length encoding is not supported in DER"); | |
| } | |
| if (length > 127) { | |
| int size = length & 0x7f; | |
| // Note: The invalid long form "0xff" (see X.690 8.1.3.5c) will be caught here | |
| if (size > 4) { | |
| throw new IOException("DER length more than 4 bytes: " + size); | |
| } | |
| length = 0; | |
| for (int i = 0; i < size; i++) { | |
| int next = read(); | |
| if (next < 0) { | |
| throw new EOFException("EOF found reading length"); | |
| } | |
| length = (length << 8) + next; | |
| } | |
| if (length < 0) { | |
| throw new IOException("corrupted stream - negative length found"); | |
| } | |
| if (length >= limit) // after all we must have read at least 1 byte | |
| { | |
| throw new IOException("corrupted stream - out of bounds length found"); | |
| } | |
| } | |
| return length; | |
| } |
🤖 Prompt for AI Agents
In
`@authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/ASN1Decoder.java`
around lines 127 - 167, The method ASN1Decoder.readLength() currently returns -1
to signal indefinite-length encoding (0x80), but callers like readSequence() and
readInteger() do not handle -1 and can misbehave; change readLength() to throw
an IOException when encountering 0x80 (indefinite-length) instead of returning
-1 so callers get a clear error path consistent with other validation checks;
update any error message to indicate "indefinite-length encoding (0x80) not
supported" and rely on existing exception handling in readSequence() and
readInteger() to avoid negative array sizes or silent empty reads.
| ASN1Encoder.create().write(rBigInteger); | ||
| ASN1Encoder.create().write(sBigInteger); |
There was a problem hiding this comment.
Dead code: two ASN1Encoder instances created and immediately discarded.
Lines 114-115 create encoders, write R and S, and throw away the results. The actual encoding is performed on lines 117-121. These are likely leftover from development.
Proposed fix — remove dead lines
BigInteger rBigInteger = new BigInteger(r);
BigInteger sBigInteger = new BigInteger(s);
- ASN1Encoder.create().write(rBigInteger);
- ASN1Encoder.create().write(sBigInteger);
-
return ASN1Encoder.create()
.writeDerSeq(
ASN1Encoder.create().write(rBigInteger),🤖 Prompt for AI Agents
In
`@authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/AuthzClientCryptoProvider.java`
around lines 114 - 115, Remove the two dead ASN1Encoder.create().write(...)
calls that discard results; in AuthzClientCryptoProvider replace the redundant
ASN1Encoder.create().write(rBigInteger) and
ASN1Encoder.create().write(sBigInteger) invocations with only the actual
encoding path that uses the encoder instance later (the code that performs the
real encoding for rBigInteger and sBigInteger), ensuring you keep the working
encoder usage and remove the discarded temporary encoder calls.
| signature.update(data); | ||
| byte[] sign = signature.sign(); | ||
| byte[] rsConcat = prov.getEcdsaCryptoProvider().asn1derToConcatenatedRS(sign, algorithm.getSignatureLength()); | ||
| byte[] asn1Des = prov.getEcdsaCryptoProvider().concatenatedRSToASN1DER(rsConcat, algorithm.getSignatureLength()); |
There was a problem hiding this comment.
Typo: asn1Des → asn1Der.
🤖 Prompt for AI Agents
In
`@authz/client/src/test/java/org/keycloak/authorization/client/test/ECDSAAlgorithmTest.java`
at line 54, Rename the misspelled variable asn1Des to asn1Der in
ECDSAAlgorithmTest.java where the result of
prov.getEcdsaCryptoProvider().concatenatedRSToASN1DER(rsConcat,
algorithm.getSignatureLength()) is assigned; update all usages of asn1Des in the
test to use asn1Der so the variable name correctly reflects the ASN.1 DER format
returned by concatenatedRSToASN1DER.
| if (foundProviders.size() > 1) { | ||
| StringBuilder builder = new StringBuilder("Ignored crypto providers: "); | ||
| for (int i = 1 ; i < foundProviders.size() ; i++) { | ||
| builder.append(foundProviders.get(i).getClass().getName() + ", "); | ||
| } | ||
| logger.debugf(builder.toString()); | ||
| } |
There was a problem hiding this comment.
logger.debugf with unsanitized format string; also trailing comma in output.
Two minor issues:
logger.debugf(builder.toString())treats the string as a format pattern. If any provider class name ever contains%sor%d, this will break or produce garbled output. Uselogger.debug(builder.toString())instead.- The loop always appends
", "after each provider name, leaving a trailing separator.
Proposed fix
if (foundProviders.size() > 1) {
- StringBuilder builder = new StringBuilder("Ignored crypto providers: ");
- for (int i = 1 ; i < foundProviders.size() ; i++) {
- builder.append(foundProviders.get(i).getClass().getName() + ", ");
- }
- logger.debugf(builder.toString());
+ List<String> ignored = foundProviders.subList(1, foundProviders.size()).stream()
+ .map(p -> p.getClass().getName())
+ .collect(Collectors.toList());
+ logger.debugf("Ignored crypto providers: %s", ignored);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (foundProviders.size() > 1) { | |
| StringBuilder builder = new StringBuilder("Ignored crypto providers: "); | |
| for (int i = 1 ; i < foundProviders.size() ; i++) { | |
| builder.append(foundProviders.get(i).getClass().getName() + ", "); | |
| } | |
| logger.debugf(builder.toString()); | |
| } | |
| if (foundProviders.size() > 1) { | |
| List<String> ignored = foundProviders.subList(1, foundProviders.size()).stream() | |
| .map(p -> p.getClass().getName()) | |
| .collect(Collectors.toList()); | |
| logger.debugf("Ignored crypto providers: %s", ignored); | |
| } |
🤖 Prompt for AI Agents
In `@common/src/main/java/org/keycloak/common/crypto/CryptoIntegration.java`
around lines 64 - 70, The logging uses logger.debugf with an unsanitized format
string and the loop leaves a trailing comma; in CryptoIntegration when handling
foundProviders, replace logger.debugf(builder.toString()) with logger.debug(...)
and build the provider list without a trailing ", " (e.g., append separators
conditionally or use String.join over
foundProviders.stream().skip(1).map(p->p.getClass().getName()).collect(...)) so
the output is printed raw and has no trailing separator.
| /** | ||
| * Order of this provider. This allows to specify which CryptoProvider will have preference in case that more of them are on the classpath. | ||
| * | ||
| * The higher number has preference over the lower number | ||
| */ | ||
| int order(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for CryptoProvider implementations
rg -nP --type=java 'implements\b.*\bCryptoProvider\b' -C2Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 2598
🏁 Script executed:
#!/bin/bash
# Check if the CryptoProvider implementations have order() method
rg -nP --type=java 'order\(\)' crypto/elytron/src/main/java/org/keycloak/crypto/elytron/WildFlyElytronProvider.java crypto/default/src/main/java/org/keycloak/crypto/def/DefaultCryptoProvider.java crypto/fips1402/src/main/java/org/keycloak/crypto/fips/FIPS1402Provider.java authz/client/src/main/java/org/keycloak/authorization/client/util/crypto/AuthzClientCryptoProvider.java -C3Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 2900
🏁 Script executed:
#!/bin/bash
# Check the CryptoProvider interface definition and its module visibility
cat common/src/main/java/org/keycloak/common/crypto/CryptoProvider.java | head -50
# Also check if there are any module-info files or access modifiers
rg -nP --type=java 'public.*interface\s+CryptoProvider' common/src/main/java/org/keycloak/common/crypto/CryptoProvider.java -B5 -A10Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 2435
Adding a non-default method to a public interface breaks external implementations.
While this is a breaking change for third-party implementations of CryptoProvider, all internal implementations in this codebase (AuthzClientCryptoProvider, DefaultCryptoProvider, FIPS1402Provider, and WildFlyElytronProvider) have already been properly updated with implementations returning their respective order values (100 or 200).
If backward compatibility with external implementations is required, provide a default implementation:
Proposed fix
- int order();
+ default int order() {
+ return 0;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Order of this provider. This allows to specify which CryptoProvider will have preference in case that more of them are on the classpath. | |
| * | |
| * The higher number has preference over the lower number | |
| */ | |
| int order(); | |
| /** | |
| * Order of this provider. This allows to specify which CryptoProvider will have preference in case that more of them are on the classpath. | |
| * | |
| * The higher number has preference over the lower number | |
| */ | |
| default int order() { | |
| return 0; | |
| } |
🤖 Prompt for AI Agents
In `@common/src/main/java/org/keycloak/common/crypto/CryptoProvider.java` around
lines 39 - 44, The new non-default method on the public interface CryptoProvider
(int order()) will break external implementations; change it to a default method
(default int order()) that returns a safe default (e.g., 0) so existing
third-party implementations remain compatible while internal classes
(AuthzClientCryptoProvider, DefaultCryptoProvider, FIPS1402Provider,
WildFlyElytronProvider) can keep their explicit overrides with 100/200.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
New Features
Tests
Chores